The first commit modifies the testbench to replicate the error originally found in Occamy.

You can see from the Gitlab CI that the first commit fails with a Fatal: Trying to pop data although the FIFO is empty error. The second corrects this behaviour (CI times out because I incremented the number of tests, but the tests are passing).

I would revert the first commit and then I believe this should be ready to merge. If the MIN_WAIT_CYCLES and MAX_WAIT_CYCLES parameters could be changed at run time then we could keep some tests also in the modified conditions, but this should probably be addressed in a separate PR anyways.

I think it would be valuable to ensure this corner case is triggered in the future to prevent this fix from being removed, so it would be great if we could figure out a way to keep this task triggering the error as part of the CI. Could we trigger this with a few individual transactions?
Other than this, I did not look at the details yet and am not super familiar with the module, but I will look through it properly.

Thanks Michael :)

It's been a long time so I have to familiarize myself with this again, but I'll look into options to keep it triggered. Probably have some notes in an old WR, but I remember it not being trivial. I'll let you know ASAP.

@micprog I've looked into this again, but it's not trivial to replicate the error in a manner which can be consistently tested by the CI.

At this point I think we should just merge this accepting the risk you highlighted. If you give me green light, I will drop the first commit which was used to reproduce the error, such that the CI does not timeout. Then we should be able to merge.

@colluca Sorry for the delay. I see what you mean. I rebased the branch on master, let's make sure the current CI is passing with this fix.

I also looked through the fix a bit more and understand what it aims to achieve. While it achieves what you are looking for, there may be some performance gain to be had by not only asserting w_valid when the forward_b_fifo is not full, but also making this conditional on the last bit of the corresponding W beat, as the B will only be returned once the last W beat is sent. This adjustment would allow almost all W beats to already be sent downstream for processing, only holding back the one that will trigger the B response that breaks the FIFO.

--- a/src/axi_dw_downsizer.sv
+++ b/src/axi_dw_downsizer.sv
@@ -736,7 +736,7 @@ module axi_dw_downsizer #(
             automatic addr_t slv_port_offset = AxiSlvPortStrbWidth == 1 ? '0 : w_req_q.aw.addr[idx_width(AxiSlvPortStrbWidth)-1:0];
 
             // Valid output
-            mst_req.w_valid = !forward_b_beat_full;
+            mst_req.w_valid = !(forward_b_beat_full && w_req_q.aw.len == 0);
             mst_req.w.last  = w_req_q.aw.len == 0;
             mst_req.w.user  = slv_req_i.w.user   ;

@micprog what you describe sounds correct to me, and the specific expression in the RTL looks correct as well. Good catch :)

@colluca I found a way to force the error by forcing significant stalls on the B channel. Please check out the testbench modification and let me know if it makes sense to you.

I will resort the commits a bit to update the TB appropriately and rebase on master